Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Include TypeScript source in npm package #190

Merged
merged 1 commit into from
Jun 21, 2016
Merged

Conversation

tomdale
Copy link
Contributor

@tomdale tomdale commented Jun 17, 2016

This is useful for TypeScript apps that want to consume Glimmer directly

@rwjblue
Copy link
Member

rwjblue commented Jun 17, 2016

Actually, I believe that this is not quite correct for consuming in an application without that app having to manually build via ember-cli-build.js (which isn't included in in files either).

There is a bit of work going on in the build to make a handlebars package (see here and here), and the rest of the project assumes it can import these from 'handlebars/*' paths (i.e. here). We also assume that you can import directly from simple-html-tokenizer in a few places...

@tomdale
Copy link
Contributor Author

tomdale commented Jun 21, 2016

@rwjblue I see your point. For my particular use case, I'm actually requiring Glimmer's ember-cli-build.js file for the build (that's another file that should probably be in the files array), and only using the TypeScript files directly for the type annotations. This is actually modeled on how TypeScript itself is set up.

Still, it seems kind of silly to "build" the TypeScript stuff into dist/. Ideally the weirdness with handlebars/* and simple-html-tokenizer can be addressed more cleanly than by patching it in in a few different, non-standard ways. I think the best bet, ideally, is to consume everything as ES6 modules and use TypeScript's paths feature (only on TS nightly at the moment) to tell the compiler where to find all the dependencies.

@tomdale
Copy link
Contributor Author

tomdale commented Jun 21, 2016

@rwjblue Given emberjs/ember.js#13615 and the fact that Ember relies on including the built-in Glimmer ember-cli-build.js, I think we should probably remove the files entry from package.json entirely. At the moment, Ember relies on being able to consume the entire package, and do its own build, from npm.

@rwjblue
Copy link
Member

rwjblue commented Jun 21, 2016

Now this will make it exclude dist/, which makes CJS not work. Can you add an .npmignore that is roughly a copy of .gitignore but not excluding dist/?

This removes the `files` entry from the `package.json` so Glimmer ships with everything other packages need to do their own build, including the TypeScript and the ember-cli-build.js file. It adds a .npmignore file, which is a copy of the .gitignore file with dist/ removed.
@rwjblue
Copy link
Member

rwjblue commented Jun 21, 2016

LGTM

@asakusuma
Copy link
Contributor

@tomdale I think you'll need to rebase

@tomdale
Copy link
Contributor Author

tomdale commented Jun 21, 2016

Looks like the build is failing, not sure why yet.

@rwjblue
Copy link
Member

rwjblue commented Jun 21, 2016

@tomdale - @asakusuma fixed (by pinning typescript version) last night, I restarted the second travis job (one passed and one failed)

@rwjblue
Copy link
Member

rwjblue commented Jun 21, 2016

YOLO

@rwjblue rwjblue merged commit 4886e29 into master Jun 21, 2016
@rwjblue rwjblue deleted the include-ts-in-npm branch June 21, 2016 20:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants